Per series roll period by evamedia · Pull Request #811 · danvk/dygraphs

evamedia · 2016-12-25T11:50:51Z

Enables adding of rollPeriod inside series parameters. If rollPeriod is
set globally, series will default to that if parameter not set

Enables adding of rollPeriod inside series parameters. If rollPeriod is set globally, series will default to that if parameter not set

coveralls · 2016-12-25T11:53:37Z

Coverage increased (+0.01%) to 90.071% when pulling adaa6ca on evamedia:per-series-roll-period into 623dd1d on danvk:master.

danvk · 2016-12-27T05:36:32Z

Thanks for the contribution! To be considered for merging, this needs unit tests and needs to follow the style of the rest of the dygraphs code.

danvk · 2016-12-30T04:35:17Z

You can absolutely update per-series options. Here's a demo: http://jsfiddle.net/eM2Mg/9155/

…

On Thu, Dec 29, 2016 at 4:37 AM, evamedia ***@***.***> wrote: Looking into unit tests, can you currently change per-series options with updateOptions() ? Limited testing seems to say no, I also found this // TODO(danvk): validate per-series options. // Supported: // strokeWidth // pointSize // drawPoints // highlightCircleSize — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#811 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF__fLLt58GIrXcmeCM0181VECUNhIvks5rM39XgaJpZM4LVZD1> .

Test’s setting of per series rollPeriod in initial setup, then changing, and adding to another series. Tests series defaults to global rollPeriod if not explicitly set

coveralls · 2016-12-30T11:07:12Z

Coverage increased (+0.09%) to 90.155% when pulling 41047fb on evamedia:per-series-roll-period into 623dd1d on danvk:master.

evamedia · 2016-12-30T11:07:48Z

apologies, auto_test submitted, can you give me a pointer on the style? Is there a guide?

Shows two series and demonstrates independent changing of rolling average

danvk · 2016-12-30T14:42:55Z

https://github.com/danvk/dygraphs/blob/master/DEVELOP.md

coveralls · 2016-12-30T14:44:07Z

Coverage increased (+0.09%) to 90.155% when pulling 8c386f1 on evamedia:per-series-roll-period into 623dd1d on danvk:master.

coveralls · 2016-12-30T15:36:34Z

Coverage increased (+0.1%) to 90.176% when pulling 717ff0b on evamedia:per-series-roll-period into 623dd1d on danvk:master.

danvk

This is looking good. I'd like one of the behaviors to be different, but other than that it's just style nits and cleanup. Thanks for adding the tests!

danvk · 2016-12-30T15:32:44Z

auto_tests/tests/per_series_rolling_average.js

+
+  var seriesOpt = {};
+  seriesOpt["Y"] = { rollPeriod: 2 };
+  g.updateOptions ({ series: seriesOpt });


Make the updateOptions calls self-contained by writing them as:

g.updateOptions({ series: { Y: { rollPeriod: 2 } } });

and killing the seriesOpt variable (here and below).

danvk · 2016-12-30T15:36:49Z

auto_tests/tests/per_series_rolling_average.js

+  g.setSelection(3); assert.equal("3: Y: 3 Z: 15", Util.getLegend());
+  assert.equal(1, g.getOption("rollPeriod", "Y"));
+  assert.equal(4, g.getOption("rollPeriod", "Z"));
+


style nit: remove these blank lines

danvk · 2016-12-30T15:36:55Z

auto_tests/tests/per_series_rolling_average.js

+    height: 320,
+	rollPeriod: 2,
+      series: {
+          Y: {


style nit: fix indentation here

danvk · 2016-12-30T15:37:23Z

auto_tests/tests/per_series_rolling_average.js

+  g.setSelection(2); assert.equal("2: Y: 1 Z: 10", Util.getLegend());
+  g.setSelection(3); assert.equal("3: Y: 2 Z: 20", Util.getLegend());
+  assert.equal(3, g.getOption("rollPeriod", "Y"));
+  assert.equal(3, g.getOption("rollPeriod", "Z"));


This should be 2 to be consistent with how other per-series options work. See http://jsfiddle.net/eM2Mg/9158/

Not sure I understand "Y" is 3 because it was set previously, "Z" is 3 because the global rollPeriod was updated and "Z" hadn't been set separately. On the example initial strokeWidth : 4 for "Z" then the global StrokeWidth is changed to 1 , leaving "X" at 2

Ah, nevermind. I misread this.

Could you add a test that makes sure updating the global rollPeriod doesn't affect a per-series rollPeriod?

danvk · 2016-12-30T15:37:51Z

src/dygraph.js

-    if (this.rollPeriod_ > 1) {
-      series = this.dataHandler_.rollingAverage(series, this.rollPeriod_, this.attributes_);
+	var seriesRollPeriod;
+


style nit: remove blank line & fix indentation

danvk · 2016-12-30T15:38:15Z

tests/per-series-rolling-average.html

+    </style>
+  </head>
+  <body>
+


style nit: use a two space indent in this file

danvk · 2016-12-30T15:40:20Z

src/dygraph.js

+	var seriesRollPeriod;
+
+	if (this.getOption('rollPeriod', seriesName[i])) { 
+		seriesRollPeriod = this.getOption('rollPeriod', seriesName[i]);


It should suffice to say:

const rollPeriod = this.getOption('rollPeriod', seriesName[i]);

this will get the per-series value or fall back to the global one.

danvk · 2016-12-30T15:41:12Z

auto_tests/tests/per_series_rolling_average.js

+  g.setSelection(3); assert.equal("3: Y: 3 Z: 25", Util.getLegend());
+  assert.equal(1, g.getOption("rollPeriod", "Y"));
+  assert.equal(2, g.getOption("rollPeriod", "Z"));
+  assert.equal(2, g.rollPeriod());


We should get rid of the rollPeriod() method. getOption('rollPeriod') does the same thing but also lets you get per-series values.

danvk · 2016-12-30T15:42:48Z

src/dygraph.js

  for (var i = 1; i < this.numColumns(); i++) {
    // var logScale = this.attr_('logscale', i); // TODO(klausw): this looks wrong // konigsberg thinks so too.
    var series = this.dataHandler_.extractSeries(this.rawData_, i, this.attributes_);
-    if (this.rollPeriod_ > 1) {


Can you get rid of the rollPeriod_ member variable altogether?

Do you mean the whole library?

Altered last test to prove changing global rollPeriod doesn’t change a series rollPeriod

coveralls · 2016-12-30T17:16:03Z

Coverage increased (+0.09%) to 90.148% when pulling c57720f on evamedia:per-series-roll-period into 623dd1d on danvk:master.

coveralls · 2016-12-31T11:24:39Z

Coverage increased (+0.09%) to 90.148% when pulling ab6575d on evamedia:per-series-roll-period into 623dd1d on danvk:master.

coveralls · 2017-01-07T16:51:40Z

Coverage increased (+0.1%) to 90.17% when pulling 28c3e15 on evamedia:per-series-roll-period into 623dd1d on danvk:master.

mdrmdrmdr · 2018-01-03T08:43:20Z

Any idea if/when this functionality gets into the software? Currently I add it manally on each Dygraph release.

Per series roll period

adaa6ca

Enables adding of rollPeriod inside series parameters. If rollPeriod is set globally, series will default to that if parameter not set

Added auto_test

41047fb

Test’s setting of per series rollPeriod in initial setup, then changing, and adding to another series. Tests series defaults to global rollPeriod if not explicitly set

Adding test html example

8c386f1

Shows two series and demonstrates independent changing of rolling average

Style guide updates

717ff0b

danvk requested changes Dec 30, 2016

View reviewed changes

Changes Requested

c57720f

Altered last test to prove changing global rollPeriod doesn’t change a series rollPeriod

removed a few tabs

ab6575d

var not const

28c3e15

mdrmdrmdr mentioned this pull request Jan 25, 2024

V2.2.1 not a drop in replacement for V2.1.0. 2nd series drawn wrong in my case #1045

Closed

Conversation

evamedia commented Dec 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 25, 2016

Uh oh!

danvk commented Dec 27, 2016

Uh oh!

danvk commented Dec 30, 2016 via email

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

evamedia commented Dec 30, 2016

Uh oh!

danvk commented Dec 30, 2016

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

danvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

coveralls commented Dec 31, 2016

Uh oh!

coveralls commented Jan 7, 2017

Uh oh!

mdrmdrmdr commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

evamedia commented Dec 25, 2016 •

edited

Loading

mdrmdrmdr commented Jan 3, 2018 •

edited

Loading